-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support resolving environment variable references in matrix config #7682
Conversation
cbe6d8c
to
b94e9e9
Compare
$importPath = $_.Value | ||
} | ||
} | ||
if ((!$matrix -and !$importPath) -or !$importPath) { | ||
return $matrix, @(), @{} | ||
return $matrix, @(), $displayNamesLookup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also fixed a separate bug here where display names lookup was getting lost if we didn't have imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have a test-branch where you're actually trying this, but this code update looks fine to me. Nothing is jumping out at me 😆
@mikeharder for example, this is what I'm thinking of switching our matrix configs to:
Then we can define all these values in a single place like |
70aabf9
to
52e3eda
Compare
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
$envKey = $flattened.Value.Replace("env:", "") | ||
$value = [System.Environment]::GetEnvironmentVariable($envKey) ?? "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$envKey = $flattened.Value.Replace("env:", "") | |
$value = [System.Environment]::GetEnvironmentVariable($envKey) ?? "" | |
$value = (Get-Item $flattened.Value -ErrorAction SilentlyContinue)?.Value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me the code is a bit less clear if you don't know how get-item works, and there could be issues in the future if the outer conditional gets removed, allowing file lookups, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just trying to avoid us needing to parse variables when PS already does that. In this case it is pretty simple so I'm not too worried about splitting and just getting the env variable directly. I'll leave it to you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple suggestions but looks good otherwise.
The following pipelines have been queued for testing: |
c46c2cf
to
5b9cb4a
Compare
The following pipelines have been queued for testing: |
Closing this for now as we don't need this feature currently. |
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#7682 See [eng/common workflow](https://github.com/Azure/azure-sdk-tools/blob/main/eng/common/README.md#workflow) --------- Co-authored-by: Ben Broderick Phillips <[email protected]>
sorry for all the formatting updates, vscode got more opinionated this time and I don't want to fight it
This adds the ability to reference environment variables to be resolved at runtime for matrix values, for example:
The primary use case is so we can reference common values like
LinuxPool
that we define as top level variables in an azure pipelines context, so we don't need to repetitively hardcode them in all the matrix configs like we do today.